Skip to content

fix bytevector-ieee-double-native-set! error#1025

Open
gus-massa wants to merge 1 commit intocisco:mainfrom
gus-massa:26-3-BytevectorSet
Open

fix bytevector-ieee-double-native-set! error#1025
gus-massa wants to merge 1 commit intocisco:mainfrom
gus-massa:26-3-BytevectorSet

Conversation

@gus-massa
Copy link
Copy Markdown
Contributor

When the last argument raises an error and the offset is out of the bounds, the compiler was reorganizing the code and the out of bound error was raised instead.

(define bv (make-bytevector 16 0))
(define (f x) (bytevector-ieee-double-native-set! bv 100 (+ x 1)))
(f 'bad)

I think it's isolated enough to be fixed now, but it's more tricky than what I expected.

It's almost a single line change, but I'm not sure if I'm using correctly the flags of bind to ensure that the last argument of #2%bytevector-ieee-double-native-set! is correctly handled when it's an unboxed flonum. Also, I think once the temporary variable is marked as fp then #3%bytevector-ieee-double-native-set! will not expand to $real->flonum another time, just use the variable.

@burgerrg burgerrg changed the title fFx bytevector-ieee-double-native-set! error fix bytevector-ieee-double-native-set! error Mar 6, 2026
When the last argument raises an error and the offset is out of
the bounds, the compiler was reorganizing the code and the
out of bound error was raised instead.

  (define bv (make-bytevector 16 0))
  (define (f x) (bytevector-ieee-double-native-set! bv 100 (+ x 1)))
  (f 'bad)
@gus-massa gus-massa force-pushed the 26-3-BytevectorSet branch from cab4dcf to d8b918b Compare March 12, 2026 02:10
@gus-massa
Copy link
Copy Markdown
Contributor Author

[Rebase after version and submodule change. Not changes in my PR code.]

@mflatt
Copy link
Copy Markdown
Contributor

mflatt commented Mar 13, 2026

The flags to bind look right here, but wrapping build-$real->flonum around the last argument's expression means that (bytevector-ieee-double-native-set! bv 100 "oops") complains about "oops" first, instead of complaining about the index 100 (while other bytevector-....-set! procedures would complain about the index before checking the value to install).

How about introducing maybe-fp to bind, and using that to enable unboxing when e-val is known to produce a flonum, leaving build-$real->flonum to #3%bytevector-ieee-double-native-set!? Concretely: mflatt@5f50d6c

We should also add regression tests for the original problem and to check that (bytevector-ieee-double-native-set! bv 100 "oops") preserves its error (where both the index and value are wrong).

For release notes, how about this rephrasing?

Optimization of a call to \scheme{bytevector-ieee-double-native-set!} was fixed to always evaluate its last argument before checking whether the index argument is in bounds.

@gus-massa
Copy link
Copy Markdown
Contributor Author

For some reason I was assuming that #3% version do not raise an error under normal conditions, they just crash or give a nonsensical result or give the correct result by chance or they even may raise an unexpected error or even the correct error, but you should not expect them to raise the correct error.

This is wrong, but perhaps it's correct for some specific cases like #3$exact?, but not in general. (I made a fix in the sibling PR just in case. ) Here I was trying to avoid raising an error in bytevector-ieee-double-native-set!!, but it's not necessary.

I agree with your changes. I guess it does not break flonum unboing but I'm not sure if it's possible to "see" it using something similar to expand/optimize.


A minor comment, is that I don't like maybe-fp because I interpret it as (or (flonum? x) (not x)). In this context my interpretation of maybe-fp makes no sense, and I don't have a better name for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants